Fix the tests on cyclonedds by translating qos duration values #606

emersonknapp · 2021-01-21T01:19:08Z

Makes the build start passing again - it has been failing since https://github.com/ros2/rosbag2/actions/runs/479072133 on 1/11 due to ros2/ros2#1058

Should unblock other open PRs - notably #603 #543 #605

Here's the full breakdown:

Providing 0 as input duration to a publisher results in 'unspecified' for all rmw implementations for all currently supported QoS policies
FastDDS reports this special value (2147483647, 4294967295) when you query a publisher that has an 'unspecified' duration
- FastDDS accepts this value as 'unspecified' when creating a publisher (the same as it does 0) - but it WON'T parse the CycloneDDS value
CycloneDDS reports the special value (9223372036, 854775807) when you query a publisher that has an 'unspecified' duration
- CycloneDDS accepts this value as 'unspecified' when creating a publisher (the same as it does 0) - but it WON'T parse the FastDDS value

The rmw_cyclonedds_cpp error looks like this when trying to read a bag recorded with rmw_fastrtps_cpp

2021-01-12T01:40:46.2371892Z 2: 1610415583.125080 [0] dq.builtin: invalid parameter list (vendor 1.16, version 2.1): pid 23 (DEADLINE) invalid, input = 3,0,0,128,6,250,130,75
2021-01-12T01:40:46.2372518Z 2: 1610415583.125090 [0] dq.builtin: SPDP (vendor 1.16): invalid qos/parameters
2021-01-12T01:40:46.2373131Z 2: 1610415583.126850 [0] dq.builtin: invalid parameter list (vendor 1.16, version 2.1): pid 23 (DEADLINE) invalid, input = 3,0,0,128,6,250,130,75
2021-01-12T01:40:46.2373762Z 2: 1610415583.126857 [0] dq.builtin: SPDP (vendor 1.16): invalid qos/parameters

The rmw_fastrtps_cpp error looks like this when trying to read a bag recorded with rmw_cyclonedds_cpp

[WARN] [1611257485.439465477] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.439495434] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.439502850] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.440722168] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.440737894] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.440758600] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.441663946] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX
[WARN] [1611257485.441677598] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX

This is a bad situation - I think we need to definitively resolve ros2/rmw#255 in order to actually solve this problem. It seems I may need to bump up the priority on that.

FOR TODAY - I think the unblocking resolution is to just make the test-sample data use "0" across the board, which can be parsed and tested on any implementation - and track the fact that QoS profiles aren't cross-implementation parseable.

emersonknapp · 2021-01-21T08:02:17Z

Gist: https://gist.githubusercontent.com/emersonknapp/719356394006587cf55d28e9563f2153/raw/cf46bf7027e033806bf3fde565a665781f01bcae/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_py rosbag2_transport rosbag2_tests
TEST args: --packages-select ros2bag rosbag2_py rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7494

Linux
Linux-aarch64
macOS
Windows

rosbag2_transport/src/rosbag2_transport/qos.cpp

ros2bag/test/resources/empty_bag/metadata.yaml

Karsten1987

I approve this given that it potentially unblocks other PRs. However, there's a lot of magic going on here and I would love to have this documented somehow. I feel this is all going to be pretty confusing for me if I look at this again in 6 months.

Karsten1987 · 2021-01-21T19:08:50Z

rosbag2_transport/src/rosbag2_transport/qos.cpp

+namespace
+{
+// These values are returned to mean "unspecified" in FastDDS
+// 0 is equivalent and is portable to other implementation


If 0 is equivalent, why do we need this change?
I feel generally we should document this in a more central way - also on the actual description of this PR for future references.

Here's the full breakdown:

Providing 0 as input duration to a publisher results 'unspecified' for all rmw implementations for all currently supported QoS policies

FastDDS reports this special value (2147483647, 4294967295) when you query a publisher that has an 'unspecified' duration

FastDDS accepts this value as 'unspecified' when creating a publisher (the same as it does 0) - but it WON'T parse the CycloneDDS value

CycloneDDS reports the special value (9223372036, 854775807) when you query a publisher that has an 'unspecified' duration

CycloneDDS accepts this value as 'unspecified' when creating a publisher (the same as it does 0) - but it WON'T parse the FastDDS value

The rmw_cyclonedds_cpp error looks like this when trying to read a bag recorded with rmw_fastrtps_cpp

2021-01-12T01:40:46.2371892Z 2: 1610415583.125080 [0] dq.builtin: invalid parameter list (vendor 1.16, version 2.1): pid 23 (DEADLINE) invalid, input = 3,0,0,128,6,250,130,75 2021-01-12T01:40:46.2372518Z 2: 1610415583.125090 [0] dq.builtin: SPDP (vendor 1.16): invalid qos/parameters 2021-01-12T01:40:46.2373131Z 2: 1610415583.126850 [0] dq.builtin: invalid parameter list (vendor 1.16, version 2.1): pid 23 (DEADLINE) invalid, input = 3,0,0,128,6,250,130,75 2021-01-12T01:40:46.2373762Z 2: 1610415583.126857 [0] dq.builtin: SPDP (vendor 1.16): invalid qos/parameters

The rmw_fastrtps_cpp error looks like this when trying to read a bag recorded with rmw_cyclonedds_cpp

[WARN] [1611257485.439465477] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.439495434] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.439502850] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.440722168] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.440737894] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.440758600] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.441663946] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX [WARN] [1611257485.441677598] [rmw_dds_common]: nanoseconds value too large for 32-bits, saturated at UINT_MAX

On this further investigation, it's actually worse than I thought - I think we need to definitively resolve ros2/rmw#255 in order to actually solve this problem.

FOR TODAY - I think the unblocking resolution would be to revert any special-case code here and just make the test-sample data use "0" across the board, which can be parsed and tested on any implementation

Edit: I updated the PR and the description, according to this analysis

awesome, thanks for the clarification.

ros2bag/test/resources/incomplete_qos_duration.yaml

…o 0 for cross-implementation readability, pending a clarification of the rmw qos duration api Signed-off-by: Emerson Knapp <[email protected]>

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-01-21T20:04:46Z

Linux
Linux-aarch64
macOS
Windows

* Change all 'fastrtps-unspecified' duration values in test resources to 0 for cross-implementation readability, pending a clarification of the rmw qos duration api Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp requested a review from a team as a code owner January 21, 2021 01:19

emersonknapp requested review from Karsten1987 and jikawa-az and removed request for a team January 21, 2021 01:19

emersonknapp force-pushed the emersonknapp/fix-metadata-durations-cyclone branch 2 times, most recently from 38ae3c1 to 7608a8d Compare January 21, 2021 02:41

emersonknapp requested review from mjeronimo and prajakta-gokhale January 21, 2021 17:46

jikawa-az approved these changes Jan 21, 2021

View reviewed changes

rosbag2_transport/src/rosbag2_transport/qos.cpp Outdated Show resolved Hide resolved

prajakta-gokhale approved these changes Jan 21, 2021

View reviewed changes

david-prody reviewed Jan 21, 2021

View reviewed changes

rosbag2_transport/src/rosbag2_transport/qos.cpp Outdated Show resolved Hide resolved

zmichaels11 approved these changes Jan 21, 2021

View reviewed changes

ros2bag/test/resources/empty_bag/metadata.yaml Show resolved Hide resolved

david-prody approved these changes Jan 21, 2021

View reviewed changes

Karsten1987 approved these changes Jan 21, 2021

View reviewed changes

emersonknapp force-pushed the emersonknapp/fix-metadata-durations-cyclone branch from 7608a8d to e35b731 Compare January 21, 2021 19:40

emersonknapp requested a review from Karsten1987 January 21, 2021 19:41

Change all 'fastrtps-unspecified' duration values in test resources t…

e55fa98

…o 0 for cross-implementation readability, pending a clarification of the rmw qos duration api Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/fix-metadata-durations-cyclone branch from e35b731 to e55fa98 Compare January 21, 2021 19:42

Add comments to the human-read places

5ef32b9

Signed-off-by: Emerson Knapp <[email protected]>

jikawa-az approved these changes Jan 21, 2021

View reviewed changes

david-prody approved these changes Jan 21, 2021

View reviewed changes

prajakta-gokhale approved these changes Jan 21, 2021

View reviewed changes

emersonknapp merged commit de11589 into master Jan 21, 2021

delete-merged-branch bot deleted the emersonknapp/fix-metadata-durations-cyclone branch January 21, 2021 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the tests on cyclonedds by translating qos duration values #606

Fix the tests on cyclonedds by translating qos duration values #606

emersonknapp commented Jan 21, 2021 •

edited

Loading

emersonknapp commented Jan 21, 2021 •

edited

Loading

Karsten1987 left a comment

Karsten1987 Jan 21, 2021

emersonknapp Jan 21, 2021 •

edited

Loading

Karsten1987 Jan 21, 2021

emersonknapp commented Jan 21, 2021

Fix the tests on cyclonedds by translating qos duration values #606

Fix the tests on cyclonedds by translating qos duration values #606

Conversation

emersonknapp commented Jan 21, 2021 • edited Loading

emersonknapp commented Jan 21, 2021 • edited Loading

Karsten1987 left a comment

Choose a reason for hiding this comment

Karsten1987 Jan 21, 2021

Choose a reason for hiding this comment

emersonknapp Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Karsten1987 Jan 21, 2021

Choose a reason for hiding this comment

emersonknapp commented Jan 21, 2021

emersonknapp commented Jan 21, 2021 •

edited

Loading

emersonknapp commented Jan 21, 2021 •

edited

Loading

emersonknapp Jan 21, 2021 •

edited

Loading